-
-
Notifications
You must be signed in to change notification settings - Fork 667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PORT] hr_employee_transfer #216
Conversation
@tarzan0820 i've create a PR to your branch to fix few minor things. I've found several exception also. Will push it again after testing. |
@tarzan0820 i think it's better if:
What do you think? |
i've push another commit to your branch that fix:
|
@andhit-r I agree with making those fields readonly can you please do in a PR? |
Fix PEP8 & Pylint. Use simple header. Use OCA icon
default = { | ||
'job_id': job_id, | ||
'date_start': effective_date, | ||
'name': False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tarzan0820 this code will produce error unless we depend this module to hr_contract_reference. What do you think we should do:
- assign default value for name (e.g. /)
- depend to hr_contract_reference so that module will automatically create sequence for new contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a tough one; but I always think the less the dependencies the better:
Also this block hr_contract_reference will eventually assign a ref to the contract if we got with option 1
if vals.get('number', '/') == '/': vals['name'] = self.env['ir.sequence'].next_by_code('contract.ref')
I say let's go with option 1
Hi @tarzan0820 , i'm sorry i think i can not continue to help on this PR. This is because i believe that we are going to need a generic models to handle employee career transition (e.g: mutation, promotion, employement status change, retirement, etc). With this PR, eventually we are going to need a models for each needs. I already saw hr_employe_termination on PR list. This approach, IMHO, will give use problems when we want to make career transition history. I will submit my PR regarding this matter in the near future. |
@andhit-r understandable and thanks for the help; tag me in when you submit the PR in the new future and I will help with it |
@tarzan0820 thank you for your understanding. I tag you on several PR. I will be grateful if you can help review it. Thanks in advance. |
Please reopen the PR if it's important to you. |
Update pending merges to add .travis.yml build except
@andhit-r this is one of the dependencies of hr_infraction